refactor(backup): convert sync fs operations to async promises#235
refactor(backup): convert sync fs operations to async promises#235BillChirico wants to merge 9 commits intomainfrom
Conversation
Addresses TODO in backup.js to switch from sync fs operations to fs.promises for better performance and non-blocking I/O. - Convert getBackupDir, createBackup, parseBackupMeta to async - Replace existsSync/mkdirSync with access/mkdir promises - Replace writeFileSync/statSync with async equivalents
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| src/modules/backup.js | Converted sync fs operations to async, but startScheduledBackups doesn't await async calls - will cause unhandled promise rejections |
| src/api/routes/backup.js | Properly awaits all async backup operations in route handlers |
| src/constants/time.js | New file with centralized time constants - well structured and documented |
| src/utils/permissions.js | Implements guild-scoped admin roles feature - has tests and resolves TODO |
Last reviewed commit: b7d14d5
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (9)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/api/routes/backup.js
Line: 188
Comment:
`listBackups()` is now async but not awaited here - will return a Promise instead of the actual backup list
```suggestion
async (_req, res) => {
const backups = await listBackups();
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/api/routes/backup.js
Line: 234
Comment:
`createBackup()` is now async but not awaited here - will return a Promise instead of backup metadata
```suggestion
const meta = await createBackup();
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/api/routes/backup.js
Line: 232
Comment:
Route handler needs to be async since `createBackup()` is now async
```suggestion
async (_req, res) => {
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/api/routes/backup.js
Line: 285
Comment:
`readBackup()` is now async but not awaited here - will return a Promise instead of the backup payload
```suggestion
const payload = await readBackup(id);
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/api/routes/backup.js
Line: 281
Comment:
Route handler needs to be async since `readBackup()` is now async
```suggestion
async (req, res) => {
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/api/routes/backup.js
Line: 442
Comment:
`pruneBackups()` is now async but not awaited here - will return a Promise instead of the deleted array
```suggestion
const deleted = await pruneBackups(retention);
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/api/routes/backup.js
Line: 423
Comment:
Route handler needs to be async since `pruneBackups()` is now async
```suggestion
async (req, res) => {
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/api/routes/backup.js
Line: 187
Comment:
Route handler needs to be async since `listBackups()` is now async
```suggestion
async (_req, res) => {
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: tests/modules/backup.test.js
Line: 195
Comment:
`createBackup()` is now async - all test calls need to be awaited and test functions marked async
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Pull request overview
This PR modernizes several parts of the codebase by moving config-backup file I/O to async fs/promises, expanding guild-admin permission logic to support per-guild admin role arrays, and centralizing commonly used time constants to remove repeated “magic number” duration calculations.
Changes:
- Refactor
src/modules/backup.jsto use asyncnode:fs/promisesAPIs for non-blocking backup operations. - Add centralized time constants (
src/constants/*) and migrate multiple modules/middlewares to use them. - Extend
isGuildAdminto supportconfig.permissions.adminRolesand add tests for the new behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/permissions.test.js | Adds test coverage for guild-scoped admin roles (adminRoles) and related precedence behavior. |
| src/utils/permissions.js | Implements adminRoles support in isGuildAdmin while retaining fallback to global admin checks. |
| src/modules/backup.js | Converts backup directory ops, backup creation/listing/reading/pruning to async promise-based FS calls. |
| src/constants/time.js | Introduces centralized millisecond duration constants and common presets. |
| src/constants/index.js | Re-exports constants for consistent import paths across the codebase. |
| src/utils/duration.js | Replaces inline time arithmetic with centralized constants. |
| src/utils/guildSpend.js | Replaces default 24h window magic number with MS_PER_DAY. |
| src/utils/cache.js | Uses centralized minute constant for cleanup interval and clarifies cache-size constant placement. |
| src/api/middleware/rateLimit.js | Uses centralized RATE_LIMIT_WINDOW.SHORT default. |
| src/api/middleware/redisRateLimit.js | Uses centralized RATE_LIMIT_WINDOW.SHORT default. |
| TASK.md | Adds a task doc for centralizing time constants (example naming currently diverges from implementation). |
| tasks/task-001.md | Adds planning doc for refactoring large guilds.js route file. |
| tasks/task-002.md | Adds planning doc for adding missing tests across several modules. |
| CODE_IMPROVEMENTS.md | Adds codebase analysis notes (includes TODO references that are now outdated given this PR’s changes). |
Comments suppressed due to low confidence (1)
src/modules/backup.js:281
readBackupis now async, butrestoreBackupstill calls it withoutawait(currentlyconst payload = readBackup(id, backupDir)), sovalidateImportPayloadends up validating a Promise and restores will fail. UpdaterestoreBackup(and any other callers) toawait readBackup(...)and handle errors accordingly.
export async function readBackup(id, backupDir) {
// Validate ID against strict pattern: backup-YYYY-MM-DDTHH-mm-ss-SSS-NNNN
const BACKUP_ID_PATTERN =
/^backup-[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}-[0-9]{2}-[0-9]{2}-[0-9]{3}-[0-9]{4}$/;
if (!BACKUP_ID_PATTERN.test(id)) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR introduces centralized time constants to eliminate magic numbers, updates rate-limiting and utility modules to use these constants, migrates backup.js from synchronous to asynchronous file operations, extends permission checks with guild-scoped admin role support, and includes planning documentation and new test coverage. Changes
Possibly related PRs🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/modules/backup.js (2)
405-410:⚠️ Potential issue | 🔴 CriticalHandle async backup job errors correctly in scheduler.
createBackup()andpruneBackups()are async, but the interval callback is sync and does not await them. The current try/catch will not catch promise rejections, causing unhandled errors.As per coding guidelines: "Always use async/await for asynchronous operations and promise handling".Proposed fix
- scheduledBackupInterval = setInterval(() => { - try { - createBackup(backupDir); - pruneBackups(retention, backupDir); - } catch (err) { - logError('Scheduled backup failed', { error: err.message }); - } - }, intervalMs); + scheduledBackupInterval = setInterval(async () => { + try { + await createBackup(backupDir); + await pruneBackups(retention, backupDir); + } catch (err) { + logError('Scheduled backup failed', { error: err.message }); + } + }, intervalMs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/backup.js` around lines 405 - 410, The interval callback is calling async functions createBackup and pruneBackups without awaiting them, so promise rejections escape the try/catch; change the scheduledBackupInterval callback to an async function, await createBackup(backupDir) and await pruneBackups(retention, backupDir) inside a try/catch, and log err (or err.message) in the catch to ensure rejected promises are handled; keep the scheduledBackupInterval variable and existing logging (logError('Scheduled backup failed', { error: ... })) but move it into the async catch block.
312-320:⚠️ Potential issue | 🔴 CriticalAwait
readBackupin restore flow.Line 312 stores a Promise instead of the parsed payload. Validation/import then operate on the wrong type, and read errors become unhandled rejections.
As per coding guidelines: "Always use async/await for asynchronous operations and promise handling".Proposed fix
export async function restoreBackup(id, backupDir) { - const payload = readBackup(id, backupDir); + const payload = await readBackup(id, backupDir); const validationErrors = validateImportPayload(payload); if (validationErrors.length > 0) { throw new Error(`Invalid backup format: ${validationErrors.join(', ')}`); } info('Restoring config from backup', { id }); const result = await importConfig(payload);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/backup.js` around lines 312 - 320, The code assigns a Promise to payload by calling readBackup(id, backupDir) without awaiting it; change the restore flow to await the async readBackup call so payload contains the parsed data (i.e., replace the direct call with an awaited result), then keep the subsequent calls to validateImportPayload(payload) and await importConfig(payload) so validation/import run on the actual object and read errors are properly propagated/handled.src/api/middleware/rateLimit.js (1)
15-16:⚠️ Potential issue | 🟡 MinorUpdate JSDoc to reflect the new default constant.
Line 21 now defaults
windowMstoRATE_LIMIT_WINDOW.SHORT, but the JSDoc still states900000. Please align the documented default with the implementation.Also applies to: 21-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/middleware/rateLimit.js` around lines 15 - 16, Update the JSDoc for the rate limit middleware to match the actual default constant: change the documented default for options.windowMs from the hardcoded "900000" to reference RATE_LIMIT_WINDOW.SHORT (or describe its semantic value), so the comment aligns with the implementation where windowMs defaults to RATE_LIMIT_WINDOW.SHORT; locate the JSDoc block above the rate limit middleware export/constructor in src/api/middleware/rateLimit.js and update the `@param` line for options.windowMs (and adjust any related `@param` description for options.max if needed) to reference the RATE_LIMIT_WINDOW.SHORT default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CODE_IMPROVEMENTS.md`:
- Around line 1-176: Normalize the markdown in CODE_IMPROVEMENTS.md by fixing
heading spacing and removing trailing spaces: ensure each heading (e.g., "## 🔴
Critical Issues", "## 🟠 High Priority", "## 🟡 Medium Priority", "## 🟢 Low
Priority / Polish", "## 📊 Metrics Summary", "## 🎯 Recommended Actions
(Priority Order)") is surrounded by the proper single blank line as required by
MD022 and strip any trailing whitespace characters (MD009) throughout the file;
run a markdown linter or formatter (markdownlint/prettier) over
CODE_IMPROVEMENTS.md to automatically apply these corrections and re-run the
linter to confirm no remaining violations.
In `@src/modules/backup.js`:
- Around line 41-45: Replace the race-prone access-then-mkdir pattern by
creating the directory unconditionally: remove the try { await access(backupDir,
constants.F_OK); } catch { await mkdir(backupDir, { recursive: true }); } block
and call mkdir(backupDir, { recursive: true }) directly so the directory is
created atomically (recursive mode is no-op if it already exists); keep error
handling only if mkdir itself rejects for unexpected reasons and reference the
backupDir, access, mkdir and constants.F_OK symbols when locating the code to
change.
In `@src/utils/guildSpend.js`:
- Around line 20-21: Update the JSDoc default values so they reference the
centralized MS_PER_DAY constant instead of the hard-coded 86400000: change the
windowMs param docs (the JSDoc for the function that accepts windowMs) to
indicate the default is MS_PER_DAY and update the second occurrence mentioned
(lines 56-57) as well; search for the windowMs parameter and replace the numeric
default in the comment with a reference to MS_PER_DAY so the docs match the
implementation.
In `@src/utils/permissions.js`:
- Around line 129-149: The isGuildAdmin function currently repeats owner/Admin
checks that isAdmin already performs; remove the duplicate checks (calls to
isBotOwner and member.permissions?.has(PermissionFlagsBits.Administrator)) from
isGuildAdmin so it only evaluates guild-scoped adminRoles
(config?.permissions?.adminRoles) and, if no role match, delegates to
isAdmin(member, config) for the global/owner/Administrator logic; update
isGuildAdmin to first return true on an adminRoles match and otherwise return
isAdmin(...) so the owner/Admin checks run only once in isAdmin.
In `@TASK.md`:
- Around line 1-74: TASK.md has markdown style linter failures: add a blank line
before and after each heading (lines referencing headings in the file) and
remove trailing spaces on the final lines (currently lines 69-70); open TASK.md
and ensure all headings (e.g., "Task: Centralize Magic Numbers and Time
Constants" and subsequent section headings) are surrounded by a single blank
line above and below, delete any trailing whitespace at the end of the file, run
the markdown linter and fix any remaining heading/spacing warnings so the file
passes linting without changing content.
- Around line 39-43: Documentation shows RATE_LIMIT while the implementation
exports RATE_LIMIT_WINDOW—make them consistent by either renaming the exported
constant RATE_LIMIT_WINDOW to RATE_LIMIT in the implementation (and updating all
imports that reference RATE_LIMIT_WINDOW) or by changing the docs/example to
reference RATE_LIMIT_WINDOW (and adjust example code to use the exported values
SHORT, MEDIUM, LONG); ensure the exported symbol name (RATE_LIMIT or
RATE_LIMIT_WINDOW) and all usages/imports and the example block match exactly.
In `@tasks/task-001.md`:
- Around line 1-65: The markdown has lint violations around heading spacing and
fenced code block spacing (e.g. the top-level heading "Task 001: Refactor
guilds.js God Route" and the fenced diff block); fix by ensuring a single blank
line before and after each heading and a blank line before and after fenced code
blocks, normalize fence spacing (```), and remove any inline spacing errors so
headings and fences conform to markdownlint rules; update the file content
accordingly while preserving all text and diffs.
In `@tasks/task-002.md`:
- Around line 1-70: Edit tasks/task-002.md to satisfy markdownlint by adding
required blank lines before and after top-level and sub-headings (e.g., around
"Task 002: Add Missing Tests", "Parent", "Context", "Files to Create",
"Requirements", and each subsection heading) and remove any trailing spaces at
line ends (fix the trailing spaces near the end of the file). Ensure there is a
single blank line between paragraphs and headings and no extraneous whitespace
so heading-spacing and trailing-space rules pass.
In `@tests/utils/permissions.test.js`:
- Around line 424-442: The test must also assert that when adminRoles is present
it short-circuits and does not check adminRoleId; update the 'should prioritize
adminRoles over adminRoleId when both are set' test to include an assertion that
member.roles.cache.has was NOT called with the fallback adminRoleId (e.g.
expect(member.roles.cache.has).not.toHaveBeenCalledWith('global-admin-role')),
referencing the isGuildAdmin call and the config.permissions.adminRoleId value
to verify adminRoleId isn't checked.
---
Outside diff comments:
In `@src/api/middleware/rateLimit.js`:
- Around line 15-16: Update the JSDoc for the rate limit middleware to match the
actual default constant: change the documented default for options.windowMs from
the hardcoded "900000" to reference RATE_LIMIT_WINDOW.SHORT (or describe its
semantic value), so the comment aligns with the implementation where windowMs
defaults to RATE_LIMIT_WINDOW.SHORT; locate the JSDoc block above the rate limit
middleware export/constructor in src/api/middleware/rateLimit.js and update the
`@param` line for options.windowMs (and adjust any related `@param` description for
options.max if needed) to reference the RATE_LIMIT_WINDOW.SHORT default.
In `@src/modules/backup.js`:
- Around line 405-410: The interval callback is calling async functions
createBackup and pruneBackups without awaiting them, so promise rejections
escape the try/catch; change the scheduledBackupInterval callback to an async
function, await createBackup(backupDir) and await pruneBackups(retention,
backupDir) inside a try/catch, and log err (or err.message) in the catch to
ensure rejected promises are handled; keep the scheduledBackupInterval variable
and existing logging (logError('Scheduled backup failed', { error: ... })) but
move it into the async catch block.
- Around line 312-320: The code assigns a Promise to payload by calling
readBackup(id, backupDir) without awaiting it; change the restore flow to await
the async readBackup call so payload contains the parsed data (i.e., replace the
direct call with an awaited result), then keep the subsequent calls to
validateImportPayload(payload) and await importConfig(payload) so
validation/import run on the actual object and read errors are properly
propagated/handled.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
CODE_IMPROVEMENTS.mdTASK.mdsrc/api/middleware/rateLimit.jssrc/api/middleware/redisRateLimit.jssrc/constants/index.jssrc/constants/time.jssrc/modules/backup.jssrc/utils/cache.jssrc/utils/duration.jssrc/utils/guildSpend.jssrc/utils/permissions.jstasks/task-001.mdtasks/task-002.mdtests/utils/permissions.test.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{js,ts,jsx,tsx}: Use ESM only — Useimport/export, no CommonJS
Use single quotes for strings — No double quotes except in JSON
Always require semicolons at end of statements
Use 2-space indent, enforced by Biome
Always use async/await for asynchronous operations and promise handling
Files:
src/constants/time.jssrc/utils/permissions.jssrc/api/middleware/redisRateLimit.jssrc/utils/guildSpend.jssrc/api/middleware/rateLimit.jssrc/constants/index.jssrc/utils/duration.jssrc/utils/cache.jssrc/modules/backup.js
{src,web}/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Winston logger from
src/logger.js, NEVER useconsole.*
Files:
src/constants/time.jssrc/utils/permissions.jssrc/api/middleware/redisRateLimit.jssrc/utils/guildSpend.jssrc/api/middleware/rateLimit.jssrc/constants/index.jssrc/utils/duration.jssrc/utils/cache.jssrc/modules/backup.js
src/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use parameterized SQL queries — Never use string interpolation in database queries
Files:
src/constants/time.jssrc/utils/permissions.jssrc/api/middleware/redisRateLimit.jssrc/utils/guildSpend.jssrc/api/middleware/rateLimit.jssrc/constants/index.jssrc/utils/duration.jssrc/utils/cache.jssrc/modules/backup.js
tests/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain 80% test coverage threshold — Never lower the coverage requirement
Files:
tests/utils/permissions.test.js
src/modules/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/**/*.{js,ts}: Config section additions MUST be added toSAFE_CONFIG_KEYSinsrc/api/utils/configAllowlist.jsto enable API saves
Gate all community features behindconfig.<feature>.enabledconfiguration checks
Redis caching should usesrc/utils/cache.jsfor generic caching with Redis + in-memory fallback
Use Discord cache utilities —src/utils/discordCache.jsfor channels/roles/members,src/utils/reputationCache.jsfor leaderboard/rank data
Files:
src/modules/backup.js
🧠 Learnings (6)
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to tests/**/*.{js,ts} : Maintain 80% test coverage threshold — Never lower the coverage requirement
Applied to files:
tasks/task-002.md
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Create modules in `src/modules/` for new features, add config section to `config.json`, update `SAFE_CONFIG_KEYS`, create slash command if needed, add database migration if needed, write tests, and update dashboard UI if configurable
Applied to files:
tasks/task-001.md
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/modules/**/*.{js,ts} : Use Discord cache utilities — `src/utils/discordCache.js` for channels/roles/members, `src/utils/reputationCache.js` for leaderboard/rank data
Applied to files:
src/utils/cache.js
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/modules/**/*.{js,ts} : Redis caching should use `src/utils/cache.js` for generic caching with Redis + in-memory fallback
Applied to files:
src/utils/cache.js
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/**/*.{js,ts,jsx,tsx} : Always use async/await for asynchronous operations and promise handling
Applied to files:
src/modules/backup.js
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/modules/**/*.{js,ts} : Config section additions MUST be added to `SAFE_CONFIG_KEYS` in `src/api/utils/configAllowlist.js` to enable API saves
Applied to files:
src/modules/backup.js
🧬 Code graph analysis (3)
tests/utils/permissions.test.js (1)
src/utils/permissions.js (1)
isGuildAdmin(129-150)
src/utils/permissions.js (4)
src/utils/modAction.js (1)
config(64-64)src/api/routes/guilds.js (1)
config(658-658)src/modules/memory.js (1)
config(142-142)src/commands/config.js (3)
config(126-126)config(162-162)config(200-200)
src/modules/backup.js (2)
src/api/routes/backup.js (7)
filename(64-64)filename(286-286)payload(63-63)payload(131-131)payload(285-285)backups(188-188)retention(424-424)src/logger.js (1)
info(231-233)
🪛 GitHub Actions: CI
src/modules/backup.js
[error] 197-199: Failed to write backup file. ENOENT: no such file or directory when writing backup JSON to disk.
[error] 282-283: Invalid backup ID. readBackup threw 'Invalid backup ID' for an unknown/malformed backup identifier.
🪛 GitHub Check: CodeQL
src/modules/backup.js
[failure] 290-290: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 295-295: Potential file system race condition
The file may have changed since it was checked.
[failure] 295-295: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
🪛 GitHub Check: Test (Vitest Coverage)
src/modules/backup.js
[failure] 197-197: Unhandled error
Error: ENOENT: no such file or directory, open '/tmp/backup-test-SOq8MP/backup-2026-03-03T15-35-38-134-0004.json'
❯ Module.createBackup src/modules/backup.js:197:3
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -2, code: 'ENOENT', syscall: 'open', path: '/tmp/backup-test-SOq8MP/backup-2026-03-03T15-35-38-134-0004.json' }
This error originated in "tests/modules/backup.test.js" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
[failure] 197-197: Unhandled error
Error: ENOENT: no such file or directory, open '/tmp/backup-test-oquIA8/backup-2026-03-03T15-35-38-134-0003.json'
❯ Module.createBackup src/modules/backup.js:197:3
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -2, code: 'ENOENT', syscall: 'open', path: '/tmp/backup-test-oquIA8/backup-2026-03-03T15-35-38-134-0003.json' }
This error originated in "tests/modules/backup.test.js" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
[failure] 197-197: Unhandled error
Error: ENOENT: no such file or directory, open '/tmp/backup-test-JzclTZ/backup-2026-03-03T15-35-38-133-0002.json'
❯ Module.createBackup src/modules/backup.js:197:3
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -2, code: 'ENOENT', syscall: 'open', path: '/tmp/backup-test-JzclTZ/backup-2026-03-03T15-35-38-133-0002.json' }
This error originated in "tests/modules/backup.test.js" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
[failure] 197-197: Unhandled error
Error: ENOENT: no such file or directory, open '/tmp/backup-test-8X3CTg/backup-2026-03-03T15-35-38-093-0000.json'
❯ Module.createBackup src/modules/backup.js:197:3
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -2, code: 'ENOENT', syscall: 'open', path: '/tmp/backup-test-8X3CTg/backup-2026-03-03T15-35-38-093-0000.json' }
This error originated in "tests/modules/backup.test.js" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
The latest test that might've caused the error is "lists created backups sorted newest first". It might mean one of the following:
- The error was thrown, while Vitest was running this test.
- If the error occurred after the test had been completed, this was the last documented test before it was thrown.
[failure] 292-292: Unhandled error
Error: Backup not found: backup-2020-01-01T00-00-00-000-0000
❯ readBackup src/modules/backup.js:292:11
This error originated in "tests/modules/backup.test.js" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
[failure] 292-292: Unhandled error
Error: Backup not found: backup-2020-01-01T00-00-00-000-0000
❯ Module.readBackup src/modules/backup.js:292:11
This error originated in "tests/modules/backup.test.js" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
[failure] 292-292: Unhandled error
Error: Backup not found: backup-9999-01-01T00-00-00-000-0000
❯ Module.readBackup src/modules/backup.js:292:11
This error originated in "tests/modules/backup.test.js" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
[failure] 282-282: Unhandled error
Error: Invalid backup ID
❯ readBackup src/modules/backup.js:282:11
❯ Module.restoreBackup src/modules/backup.js:312:19
❯ tests/modules/backup.test.js:255:26
This error originated in "tests/modules/backup.test.js" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
[failure] 282-282: Unhandled error
Error: Invalid backup ID
❯ Module.readBackup src/modules/backup.js:282:11
❯ tests/modules/backup.test.js:237:18
❯ tests/modules/backup.test.js:237:55
This error originated in "tests/modules/backup.test.js" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
[failure] 282-282: Unhandled error
Error: Invalid backup ID
❯ Module.readBackup src/modules/backup.js:282:11
❯ tests/modules/backup.test.js:225:21
This error originated in "tests/modules/backup.test.js" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
🪛 LanguageTool
tasks/task-001.md
[style] ~14-~14: To elevate your writing, try using a synonym here.
Context: ...onsibility principle and makes the file hard to maintain. ## Files to Work On - `sr...
(HARD_TO)
🪛 markdownlint-cli2 (0.21.0)
tasks/task-001.md
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 4-4: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 4-4: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 21-21: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 46-46: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 52-52: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 57-57: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 62-62: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
TASK.md
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 15-15: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 24-24: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 37-37: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 44-44: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 51-51: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 58-58: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 65-65: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 69-69: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 70-70: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
CODE_IMPROVEMENTS.md
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 15-15: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 24-24: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 37-37: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 44-44: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 51-51: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 58-58: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 65-65: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 69-69: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 70-70: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🔇 Additional comments (5)
src/utils/cache.js (1)
12-55: Good centralization of cache timing constants.Using
MS_PER_MINUTEfor cleanup scheduling is a clean maintainability improvement and keeps timing behavior consistent across modules.src/api/middleware/redisRateLimit.js (1)
9-9: LGTM!The import of
RATE_LIMIT_WINDOWand its usage as the default forwindowMsis clean and aligns with the centralized constants refactor. The JSDoc comment on line 18 correctly documents the default as 900000ms which matchesRATE_LIMIT_WINDOW.SHORT.Also applies to: 23-27
src/utils/duration.js (1)
8-28: LGTM!Clean refactor replacing magic numbers with centralized constants. The behavior is preserved and the code is now more maintainable.
src/constants/index.js (1)
1-15: LGTM!Clean barrel export module for centralized constant imports. This enables cleaner import paths for consumers.
src/constants/time.js (1)
1-33: LGTM!Well-structured time constants module. Building constants incrementally (
MS_PER_MINUTE = 60 * MS_PER_SECOND) ensures consistency and makes the relationships clear. TheRATE_LIMIT_WINDOWpresets provide sensible defaults for common use cases.
| # Code Improvement Opportunities | ||
|
|
||
| Based on comprehensive analysis of the volvox-bot codebase (159 JS files, ~50k lines). | ||
|
|
||
| ## 🔴 Critical Issues | ||
|
|
||
| ### 1. Large File Refactoring Needed | ||
| **Files exceeding 500 lines need decomposition:** | ||
|
|
||
| | File | Lines | Issue | | ||
| |------|-------|-------| | ||
| | `src/api/routes/guilds.js` | 1,622 | God route - handles analytics, members, config, moderation | | ||
| | `src/api/routes/conversations.js` | 1,069 | Multiple concerns (conversations + flagged messages) | | ||
| | `src/api/routes/members.js` | 1,006 | Member management + bulk actions + reputation | | ||
| | `src/modules/events.js` | 959 | Event handler doing too much - violates SRP | | ||
| | `src/modules/config.js` | 904 | Config logic + caching + merging + validation | | ||
| | `src/modules/triage.js` | 806 | Complex AI triage - could split by stage | | ||
|
|
||
| **Recommendation:** Split into smaller modules with single responsibilities. | ||
|
|
||
| --- | ||
|
|
||
| ## 🟠 High Priority | ||
|
|
||
| ### 2. Missing Test Coverage | ||
| **Files without tests:** | ||
| - `src/modules/pollHandler.js` - No tests | ||
| - `src/modules/reputationDefaults.js` - No tests | ||
| - `src/modules/reviewHandler.js` - No tests | ||
| - `src/utils/cronParser.js` - No tests | ||
| - `src/utils/flattenToLeafPaths.js` - No tests | ||
| - `src/commands/voice.js` - Command exists but no test file | ||
|
|
||
| ### 3. TODO Items in Code | ||
| ```javascript | ||
| // src/utils/permissions.js:132 | ||
| // TODO(#71): check guild-scoped admin roles once per-guild config is implemented | ||
|
|
||
| // src/api/routes/guilds.js:1182 | ||
| // TODO(issue-122): move slash-command analytics to a dedicated usage table | ||
|
|
||
| // src/modules/backup.js:18 | ||
| // TODO: Consider switching to fs.promises for async operations | ||
| ``` | ||
|
|
||
| ### 4. Magic Numbers & Hardcoded Values | ||
| Many time constants scattered throughout: | ||
| ```javascript | ||
| // Should be configurable: | ||
| 24 * 60 * 60 * 1000 // 1 day - appears 8+ times | ||
| 15 * 60 * 1000 // 15 min - rate limit windows | ||
| 365 * 24 * 60 * 60 * 1000 // 1 year - max duration | ||
| MAX_MEMORY_CACHE_SIZE = 1000 // utils/cache.js | ||
| ``` | ||
|
|
||
| **Recommendation:** Centralize in `src/constants/time.js` or make configurable. | ||
|
|
||
| --- | ||
|
|
||
| ## 🟡 Medium Priority | ||
|
|
||
| ### 5. Error Handling Inconsistencies | ||
|
|
||
| **Inconsistent catch patterns:** | ||
| ```javascript | ||
| // Some use empty catch (swallow errors): | ||
| } catch { | ||
| // nothing | ||
| } | ||
|
|
||
| // Some log but don't rethrow: | ||
| } catch (err) { | ||
| logError('...', { error: err.message }); | ||
| } | ||
|
|
||
| // Some properly handle: | ||
| } catch (err) { | ||
| error('Failed to...', { error: err.message }); | ||
| throw err; | ||
| } | ||
| ``` | ||
|
|
||
| **Files with empty catches to review:** | ||
| - `src/utils/cache.js:87` | ||
| - `src/utils/guildSpend.js:28` | ||
| - `src/utils/debugFooter.js:298` | ||
| - `src/api/utils/sessionStore.js:71` | ||
| - `src/api/utils/webhook.js:24` | ||
| - `src/api/utils/ssrfProtection.js:204,266` | ||
| - `src/api/middleware/redisRateLimit.js:69` | ||
| - `src/api/middleware/auditLog.js:140,203,231` | ||
| - `src/api/middleware/verifyJwt.js:46,53` | ||
| - `src/api/routes/community.js:714` | ||
| - `src/api/routes/health.js:20` | ||
|
|
||
| ### 6. Potential Memory Leaks | ||
|
|
||
| **Event listeners without removal:** | ||
| - 58 `.on()` / `.once()` calls found | ||
| - Need audit of listener cleanup on shutdown/restart | ||
|
|
||
| **Timers without cleanup:** | ||
| - 55 `setTimeout` / `setInterval` instances | ||
| - Some may not be cleared on error paths | ||
|
|
||
| ### 7. Database Query Patterns | ||
|
|
||
| **Good:** All queries use parameterized inputs (no SQL injection risk) | ||
|
|
||
| **Could improve:** | ||
| - Some queries build dynamic WHERE clauses - verify all are safe | ||
| - Missing query timeout handling in some places | ||
| - No connection pool exhaustion handling visible | ||
|
|
||
| --- | ||
|
|
||
| ## 🟢 Low Priority / Polish | ||
|
|
||
| ### 8. Code Organization | ||
|
|
||
| **Import ordering inconsistent:** | ||
| Some files group by type (builtins, external, internal), others don't. | ||
|
|
||
| **Example standard:** | ||
| ```javascript | ||
| // 1. Node builtins | ||
| import { readFileSync } from 'node:fs'; | ||
|
|
||
| // 2. External packages | ||
| import { Client } from 'discord.js'; | ||
|
|
||
| // 3. Internal modules (absolute) | ||
| import { getConfig } from '../modules/config.js'; | ||
|
|
||
| // 4. Internal modules (relative) | ||
| import { helper } from './helper.js'; | ||
| ``` | ||
|
|
||
| ### 9. JSDoc Coverage | ||
|
|
||
| Many functions lack JSDoc types, making IDE support weaker. | ||
|
|
||
| ### 10. Naming Consistency | ||
|
|
||
| Some inconsistency in naming: | ||
| - `logError` vs `error` (logger imports) | ||
| - `guildId` vs `id` vs `serverId` in different contexts | ||
| - `userId` vs `user_id` (JS vs DB naming) | ||
|
|
||
| --- | ||
|
|
||
| ## 📊 Metrics Summary | ||
|
|
||
| | Metric | Count | | ||
| |--------|-------| | ||
| | Total JS files | 159 | | ||
| | Async functions | 441 | | ||
| | Await statements | 1,334 | | ||
| | Promise chains (.then/.catch) | 149 | | ||
| | Throw statements | 90 | | ||
| | New Error instances | 52 | | ||
| | Database queries | 816 | | ||
| | setTimeout/setInterval | 55 | | ||
| | Event listeners | 58 | | ||
| | TODO/FIXME comments | 3 | | ||
|
|
||
| --- | ||
|
|
||
| ## 🎯 Recommended Actions (Priority Order) | ||
|
|
||
| 1. **Split large route files** — Start with `guilds.js` (1,622 lines) | ||
| 2. **Add missing tests** — Focus on `pollHandler.js`, `reviewHandler.js` | ||
| 3. **Centralize magic numbers** — Create `src/constants/` directory | ||
| 4. **Audit error handling** — Review all empty catch blocks | ||
| 5. **Document public APIs** — Add JSDoc to exported functions | ||
| 6. **Standardize imports** — Enforce consistent ordering via lint rule |
There was a problem hiding this comment.
Resolve markdownlint violations before merge.
This file has repeated heading-spacing issues (MD022) and trailing spaces (MD009). Please normalize heading blank lines and remove trailing whitespace to avoid documentation lint failures.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 15-15: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 24-24: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 37-37: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 44-44: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 51-51: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 58-58: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 65-65: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 69-69: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 70-70: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CODE_IMPROVEMENTS.md` around lines 1 - 176, Normalize the markdown in
CODE_IMPROVEMENTS.md by fixing heading spacing and removing trailing spaces:
ensure each heading (e.g., "## 🔴 Critical Issues", "## 🟠 High Priority", "##
🟡 Medium Priority", "## 🟢 Low Priority / Polish", "## 📊 Metrics Summary", "##
🎯 Recommended Actions (Priority Order)") is surrounded by the proper single
blank line as required by MD022 and strip any trailing whitespace characters
(MD009) throughout the file; run a markdown linter or formatter
(markdownlint/prettier) over CODE_IMPROVEMENTS.md to automatically apply these
corrections and re-run the linter to confirm no remaining violations.
| try { | ||
| await access(backupDir, constants.F_OK); | ||
| } catch { | ||
| await mkdir(backupDir, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
Eliminate check-then-act race in backup directory creation.
access() followed by mkdir() is race-prone. The directory can disappear after Line 42 and before Line 197, which matches the ENOENT failures. Create the directory unconditionally with recursive mkdir.
Proposed fix
export async function getBackupDir(dir) {
const backupDir = dir ?? DEFAULT_BACKUP_DIR;
- try {
- await access(backupDir, constants.F_OK);
- } catch {
- await mkdir(backupDir, { recursive: true });
- }
+ await mkdir(backupDir, { recursive: true });
return backupDir;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/backup.js` around lines 41 - 45, Replace the race-prone
access-then-mkdir pattern by creating the directory unconditionally: remove the
try { await access(backupDir, constants.F_OK); } catch { await mkdir(backupDir,
{ recursive: true }); } block and call mkdir(backupDir, { recursive: true })
directly so the directory is created atomically (recursive mode is no-op if it
already exists); keep error handling only if mkdir itself rejects for unexpected
reasons and reference the backupDir, access, mkdir and constants.F_OK symbols
when locating the code to change.
| * @param {number} [windowMs=86400000] - Rolling window in milliseconds (default: 24 h). | ||
| * @returns {Promise<number>} Total spend in USD for the window period. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Align JSDoc defaults with centralized constants.
The implementation now defaults to MS_PER_DAY, but the JSDoc still documents 86400000. Updating docs to reference the constant keeps comments aligned with the new source of truth.
Also applies to: 56-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/guildSpend.js` around lines 20 - 21, Update the JSDoc default
values so they reference the centralized MS_PER_DAY constant instead of the
hard-coded 86400000: change the windowMs param docs (the JSDoc for the function
that accepts windowMs) to indicate the default is MS_PER_DAY and update the
second occurrence mentioned (lines 56-57) as well; search for the windowMs
parameter and replace the numeric default in the comment with a reference to
MS_PER_DAY so the docs match the implementation.
| export function isGuildAdmin(member, config) { | ||
| // TODO(#71): check guild-scoped admin roles once per-guild config is implemented | ||
| if (!member) return false; | ||
|
|
||
| // Bot owner always bypasses permission checks | ||
| if (isBotOwner(member, config)) return true; | ||
|
|
||
| // Check if member has Discord Administrator permission | ||
| if (member.permissions?.has(PermissionFlagsBits.Administrator)) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check guild-scoped admin roles (per-guild config) | ||
| const adminRoles = config?.permissions?.adminRoles; | ||
| if (Array.isArray(adminRoles) && adminRoles.length > 0) { | ||
| if (adminRoles.some((roleId) => member.roles?.cache?.has(roleId))) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| // Fall back to global admin checks (single adminRoleId, etc.) | ||
| return isAdmin(member, config); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider avoiding redundant checks in the fallback path.
isGuildAdmin checks isBotOwner (line 133) and Administrator permission (line 136) before falling back to isAdmin (line 149). However, isAdmin repeats both checks internally (lines 56 and 61). When adminRoles is empty or doesn't match, these checks execute twice.
You could either:
- Have
isGuildAdminonly checkadminRolesand delegate everything else toisAdmin, or - Add an internal variant of
isAdminthat skips the owner/Administrator checks when called fromisGuildAdmin.
This is a minor efficiency concern and the current logic is correct.
♻️ Proposed simplification
export function isGuildAdmin(member, config) {
if (!member) return false;
- // Bot owner always bypasses permission checks
- if (isBotOwner(member, config)) return true;
-
- // Check if member has Discord Administrator permission
- if (member.permissions?.has(PermissionFlagsBits.Administrator)) {
- return true;
- }
-
// Check guild-scoped admin roles (per-guild config)
const adminRoles = config?.permissions?.adminRoles;
if (Array.isArray(adminRoles) && adminRoles.length > 0) {
if (adminRoles.some((roleId) => member.roles?.cache?.has(roleId))) {
return true;
}
}
- // Fall back to global admin checks (single adminRoleId, etc.)
+ // Fall back to global admin checks (bot owner, Administrator, adminRoleId)
return isAdmin(member, config);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function isGuildAdmin(member, config) { | |
| // TODO(#71): check guild-scoped admin roles once per-guild config is implemented | |
| if (!member) return false; | |
| // Bot owner always bypasses permission checks | |
| if (isBotOwner(member, config)) return true; | |
| // Check if member has Discord Administrator permission | |
| if (member.permissions?.has(PermissionFlagsBits.Administrator)) { | |
| return true; | |
| } | |
| // Check guild-scoped admin roles (per-guild config) | |
| const adminRoles = config?.permissions?.adminRoles; | |
| if (Array.isArray(adminRoles) && adminRoles.length > 0) { | |
| if (adminRoles.some((roleId) => member.roles?.cache?.has(roleId))) { | |
| return true; | |
| } | |
| } | |
| // Fall back to global admin checks (single adminRoleId, etc.) | |
| return isAdmin(member, config); | |
| export function isGuildAdmin(member, config) { | |
| if (!member) return false; | |
| // Check guild-scoped admin roles (per-guild config) | |
| const adminRoles = config?.permissions?.adminRoles; | |
| if (Array.isArray(adminRoles) && adminRoles.length > 0) { | |
| if (adminRoles.some((roleId) => member.roles?.cache?.has(roleId))) { | |
| return true; | |
| } | |
| } | |
| // Fall back to global admin checks (bot owner, Administrator, adminRoleId) | |
| return isAdmin(member, config); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/permissions.js` around lines 129 - 149, The isGuildAdmin function
currently repeats owner/Admin checks that isAdmin already performs; remove the
duplicate checks (calls to isBotOwner and
member.permissions?.has(PermissionFlagsBits.Administrator)) from isGuildAdmin so
it only evaluates guild-scoped adminRoles (config?.permissions?.adminRoles) and,
if no role match, delegates to isAdmin(member, config) for the
global/owner/Administrator logic; update isGuildAdmin to first return true on an
adminRoles match and otherwise return isAdmin(...) so the owner/Admin checks run
only once in isAdmin.
| # Task: Centralize Magic Numbers and Time Constants | ||
|
|
||
| ## Context | ||
| The codebase has time constants scattered throughout (24 * 60 * 60 * 1000 appearing 8+ times). This makes maintenance hard and configuration impossible. | ||
|
|
||
| ## Files to Work On | ||
| - Create: `src/constants/time.js` - Time duration constants | ||
| - Create: `src/constants/index.js` - Re-export all constants | ||
| - Update files that use magic numbers: | ||
| - `src/utils/duration.js` | ||
| - `src/utils/guildSpend.js` | ||
| - `src/utils/cache.js` | ||
| - `src/api/middleware/rateLimit.js` | ||
| - `src/api/middleware/redisRateLimit.js` | ||
| - And others identified in CODE_IMPROVEMENTS.md | ||
|
|
||
| ## Requirements | ||
|
|
||
| ### Phase 1: Create Constants Module | ||
| Create `src/constants/time.js` with: | ||
| ```javascript | ||
| export const MS_PER_SECOND = 1000; | ||
| export const MS_PER_MINUTE = 60 * 1000; | ||
| export const MS_PER_HOUR = 60 * 60 * 1000; | ||
| export const MS_PER_DAY = 24 * 60 * 60 * 1000; | ||
| export const MS_PER_WEEK = 7 * 24 * 60 * 60 * 1000; | ||
| export const MS_PER_YEAR = 365 * 24 * 60 * 60 * 1000; | ||
|
|
||
| // Common durations | ||
| export const DURATION = { | ||
| MINUTE: MS_PER_MINUTE, | ||
| HOUR: MS_PER_HOUR, | ||
| DAY: MS_PER_DAY, | ||
| WEEK: MS_PER_WEEK, | ||
| YEAR: MS_PER_YEAR, | ||
| }; | ||
|
|
||
| // Rate limit windows | ||
| export const RATE_LIMIT = { | ||
| SHORT: 15 * MS_PER_MINUTE, // 15 minutes | ||
| MEDIUM: MS_PER_HOUR, | ||
| LONG: MS_PER_DAY, | ||
| }; | ||
| ``` | ||
|
|
||
| ### Phase 2: Replace Magic Numbers | ||
| Replace inline calculations with imports: | ||
| - `24 * 60 * 60 * 1000` → `MS_PER_DAY` or `DURATION.DAY` | ||
| - `15 * 60 * 1000` → `RATE_LIMIT.SHORT` | ||
| - etc. | ||
|
|
||
| ### Phase 3: Configurable Cache Sizes | ||
| Move hardcoded limits to constants: | ||
| - `MAX_MEMORY_CACHE_SIZE = 1000` in cache.js | ||
| - `MAX_ANALYTICS_RANGE_DAYS = 90` in guilds.js | ||
|
|
||
| ## Constraints | ||
| - Do NOT change any behavior - only replace constants | ||
| - Keep all tests passing | ||
| - Run lint after changes | ||
|
|
||
| ## Acceptance Criteria | ||
| - [ ] src/constants/time.js created with all time constants | ||
| - [ ] src/constants/index.js created for clean imports | ||
| - [ ] All magic number occurrences replaced | ||
| - [ ] No behavioral changes | ||
| - [ ] All tests pass | ||
| - [ ] Lint passes | ||
|
|
||
| ## Progress Tracking | ||
| Commit after each file is updated: | ||
| 1. "refactor: create centralized time constants module" | ||
| 2. "refactor: replace magic numbers in duration.js" | ||
| 3. etc. |
There was a problem hiding this comment.
Markdown formatting issues flagged by linter.
Static analysis identified several formatting issues:
- Headings should be surrounded by blank lines (lines 3, 7, 15, 24, 31, 37, 44, 51, 58, 65)
- Trailing spaces on lines 69-70
These are minor style issues but fixing them improves consistency.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 15-15: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 24-24: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 37-37: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 44-44: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 51-51: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 58-58: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 65-65: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 69-69: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 70-70: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TASK.md` around lines 1 - 74, TASK.md has markdown style linter failures: add
a blank line before and after each heading (lines referencing headings in the
file) and remove trailing spaces on the final lines (currently lines 69-70);
open TASK.md and ensure all headings (e.g., "Task: Centralize Magic Numbers and
Time Constants" and subsequent section headings) are surrounded by a single
blank line above and below, delete any trailing whitespace at the end of the
file, run the markdown linter and fix any remaining heading/spacing warnings so
the file passes linting without changing content.
| export const RATE_LIMIT = { | ||
| SHORT: 15 * MS_PER_MINUTE, // 15 minutes | ||
| MEDIUM: MS_PER_HOUR, | ||
| LONG: MS_PER_DAY, | ||
| }; |
There was a problem hiding this comment.
Plan shows RATE_LIMIT but implementation uses RATE_LIMIT_WINDOW.
The example code block shows RATE_LIMIT object but the actual implementation in src/constants/time.js uses RATE_LIMIT_WINDOW. Consider updating the documentation to match the implementation to avoid confusion for future readers.
📝 Proposed fix
-// Rate limit windows
-export const RATE_LIMIT = {
+// Rate limit window presets
+export const RATE_LIMIT_WINDOW = {
SHORT: 15 * MS_PER_MINUTE, // 15 minutes
MEDIUM: MS_PER_HOUR,
LONG: MS_PER_DAY,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const RATE_LIMIT = { | |
| SHORT: 15 * MS_PER_MINUTE, // 15 minutes | |
| MEDIUM: MS_PER_HOUR, | |
| LONG: MS_PER_DAY, | |
| }; | |
| // Rate limit window presets | |
| export const RATE_LIMIT_WINDOW = { | |
| SHORT: 15 * MS_PER_MINUTE, // 15 minutes | |
| MEDIUM: MS_PER_HOUR, | |
| LONG: MS_PER_DAY, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TASK.md` around lines 39 - 43, Documentation shows RATE_LIMIT while the
implementation exports RATE_LIMIT_WINDOW—make them consistent by either renaming
the exported constant RATE_LIMIT_WINDOW to RATE_LIMIT in the implementation (and
updating all imports that reference RATE_LIMIT_WINDOW) or by changing the
docs/example to reference RATE_LIMIT_WINDOW (and adjust example code to use the
exported values SHORT, MEDIUM, LONG); ensure the exported symbol name
(RATE_LIMIT or RATE_LIMIT_WINDOW) and all usages/imports and the example block
match exactly.
| # Task 001: Refactor guilds.js God Route | ||
|
|
||
| ## Parent | ||
| - **Master Task:** Code improvements from CODE_IMPROVEMENTS.md | ||
| - **Branch:** refactor/guilds-routes | ||
|
|
||
| ## Context | ||
| The file `src/api/routes/guilds.js` is 1,622 lines and handles too many concerns: | ||
| - Analytics endpoints (charts, stats, activity) | ||
| - Member management (list, search, bulk actions) | ||
| - Guild configuration | ||
| - Moderation actions | ||
|
|
||
| This violates single responsibility principle and makes the file hard to maintain. | ||
|
|
||
| ## Files to Work On | ||
| - `src/api/routes/guilds.js` - Source file to split | ||
| - Create: `src/api/routes/analytics.js` - Analytics endpoints | ||
| - Create: `src/api/routes/members.js` - Already exists but may need cleanup | ||
| - Create: `src/api/routes/guildConfig.js` - Config-specific routes | ||
| - Update: `src/api/index.js` - Register new routes | ||
| - Update tests as needed | ||
|
|
||
| ## Requirements | ||
|
|
||
| ### Phase 1: Extract Analytics Routes | ||
| Move these endpoints from guilds.js to analytics.js: | ||
| - `GET /:id/analytics/activity` - Message activity data | ||
| - `GET /:id/analytics/commands` - Command usage stats | ||
| - `GET /:id/analytics/voice` - Voice channel stats | ||
| - `GET /:id/analytics/overview` - Summary statistics | ||
| - `GET /:id/analytics/export` - CSV/JSON export | ||
|
|
||
| ### Phase 2: Extract Config Routes | ||
| Move these endpoints to guildConfig.js: | ||
| - `GET /:id/config` - Get guild config | ||
| - `PATCH /:id/config` - Update config | ||
| - `POST /:id/config/reset` - Reset to defaults | ||
|
|
||
| ### Phase 3: Cleanup guilds.js | ||
| After extraction, guilds.js should only handle: | ||
| - Basic guild info endpoints | ||
| - Guild validation middleware exports | ||
|
|
||
| ## Constraints | ||
| - Do NOT change API behavior - this is pure refactoring | ||
| - Keep all existing tests passing | ||
| - Export shared functions (like `parsePagination`, `parseAnalyticsRange`) from a utils file | ||
| - Update imports in index.js | ||
|
|
||
| ## Acceptance Criteria | ||
| - [ ] analytics.js created with all analytics endpoints | ||
| - [ ] guildConfig.js created with config endpoints | ||
| - [ ] guilds.js reduced to <500 lines | ||
| - [ ] All existing tests pass | ||
| - [ ] No API behavior changes | ||
| - [ ] Proper JSDoc on extracted functions | ||
| - [ ] Code passes lint check | ||
|
|
||
| ## Results | ||
| _[To be filled by subagent]_ | ||
|
|
||
| **Status:** [In Progress] | ||
| **Commits:** | ||
| **Issues:** |
There was a problem hiding this comment.
Clean up markdownlint issues in this task plan.
The file has heading-spacing/fence-spacing formatting violations that are likely to fail markdown lint checks. Please normalize spacing around headings and fenced blocks.
🧰 Tools
🪛 LanguageTool
[style] ~14-~14: To elevate your writing, try using a synonym here.
Context: ...onsibility principle and makes the file hard to maintain. ## Files to Work On - `sr...
(HARD_TO)
🪛 markdownlint-cli2 (0.21.0)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 4-4: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 4-4: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 21-21: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 46-46: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 52-52: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 57-57: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 62-62: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/task-001.md` around lines 1 - 65, The markdown has lint violations
around heading spacing and fenced code block spacing (e.g. the top-level heading
"Task 001: Refactor guilds.js God Route" and the fenced diff block); fix by
ensuring a single blank line before and after each heading and a blank line
before and after fenced code blocks, normalize fence spacing (```), and remove
any inline spacing errors so headings and fences conform to markdownlint rules;
update the file content accordingly while preserving all text and diffs.
| # Task 002: Add Missing Tests | ||
|
|
||
| ## Parent | ||
| - **Master Task:** Code improvements from CODE_IMPROVEMENTS.md | ||
| - **Branch:** refactor/missing-tests | ||
|
|
||
| ## Context | ||
| Several modules have no test coverage: | ||
| - `src/modules/pollHandler.js` - Poll voting logic | ||
| - `src/modules/reputationDefaults.js` - Default reputation config | ||
| - `src/modules/reviewHandler.js` - Review claim handling | ||
| - `src/utils/cronParser.js` - Cron expression parsing | ||
| - `src/utils/flattenToLeafPaths.js` - Object flattening utility | ||
|
|
||
| ## Files to Create | ||
| - `tests/modules/pollHandler.test.js` | ||
| - `tests/modules/reputationDefaults.test.js` | ||
| - `tests/modules/reviewHandler.test.js` | ||
| - `tests/utils/cronParser.test.js` | ||
| - `tests/utils/flattenToLeafPaths.test.js` | ||
|
|
||
| ## Requirements | ||
|
|
||
| ### pollHandler.test.js | ||
| Test the poll voting logic: | ||
| - Handle poll vote reactions | ||
| - Validate vote eligibility | ||
| - Update poll results | ||
| - Close polls and announce winners | ||
|
|
||
| ### reputationDefaults.test.js | ||
| Test default reputation configuration: | ||
| - Default XP values | ||
| - Level thresholds | ||
| - Role rewards structure | ||
|
|
||
| ### reviewHandler.test.js | ||
| Test review claim handling: | ||
| - Claim review items | ||
| - Unclaim/release reviews | ||
| - Complete reviews with feedback | ||
| - Prevent double-claiming | ||
|
|
||
| ### cronParser.test.js | ||
| Test cron expression parsing: | ||
| - Parse standard cron formats | ||
| - Handle special characters (*, /, -) | ||
| - Calculate next run times | ||
| - Error on invalid expressions | ||
|
|
||
| ### flattenToLeafPaths.test.js | ||
| Test object flattening: | ||
| - Flatten nested objects to dot paths | ||
| - Handle arrays | ||
| - Preserve primitive values | ||
| - Edge cases (null, empty objects) | ||
|
|
||
| ## Acceptance Criteria | ||
| - [ ] All 5 test files created | ||
| - [ ] Tests cover main functionality | ||
| - [ ] Tests pass (`pnpm test`) | ||
| - [ ] Coverage meets project standards (>80%) | ||
| - [ ] No lint errors | ||
|
|
||
| ## Results | ||
| _[To be filled by subagent]_ | ||
|
|
||
| **Status:** [In Progress] | ||
| **Commits:** | ||
| **Issues:** |
There was a problem hiding this comment.
Fix markdownlint issues in the task doc.
This file currently violates heading-spacing and trailing-space rules (e.g., Line 69 and Line 70). Please add required blank lines around headings and remove trailing spaces so markdownlint passes cleanly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/task-002.md` around lines 1 - 70, Edit tasks/task-002.md to satisfy
markdownlint by adding required blank lines before and after top-level and
sub-headings (e.g., around "Task 002: Add Missing Tests", "Parent", "Context",
"Files to Create", "Requirements", and each subsection heading) and remove any
trailing spaces at line ends (fix the trailing spaces near the end of the file).
Ensure there is a single blank line between paragraphs and headings and no
extraneous whitespace so heading-spacing and trailing-space rules pass.
| it('should prioritize adminRoles over adminRoleId when both are set', () => { | ||
| const member = { | ||
| permissions: { has: vi.fn().mockReturnValue(false) }, | ||
| roles: { | ||
| cache: { | ||
| has: vi.fn().mockImplementation((roleId) => roleId === 'guild-admin-role-1'), | ||
| }, | ||
| }, | ||
| }; | ||
| const config = { | ||
| permissions: { | ||
| adminRoles: ['guild-admin-role-1'], | ||
| adminRoleId: 'global-admin-role', | ||
| }, | ||
| }; | ||
| expect(isGuildAdmin(member, config)).toBe(true); | ||
| // Should match on adminRoles without checking adminRoleId | ||
| expect(member.roles.cache.has).toHaveBeenCalledWith('guild-admin-role-1'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen the “adminRoles priority” assertion.
This test should also assert that adminRoleId was not checked, otherwise the priority guarantee is only partially verified.
Proposed test enhancement
expect(isGuildAdmin(member, config)).toBe(true);
// Should match on adminRoles without checking adminRoleId
expect(member.roles.cache.has).toHaveBeenCalledWith('guild-admin-role-1');
+ expect(member.roles.cache.has).not.toHaveBeenCalledWith('global-admin-role');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/utils/permissions.test.js` around lines 424 - 442, The test must also
assert that when adminRoles is present it short-circuits and does not check
adminRoleId; update the 'should prioritize adminRoles over adminRoleId when both
are set' test to include an assertion that member.roles.cache.has was NOT called
with the fallback adminRoleId (e.g.
expect(member.roles.cache.has).not.toHaveBeenCalledWith('global-admin-role')),
referencing the isGuildAdmin call and the config.permissions.adminRoleId value
to verify adminRoleId isn't checked.
|
Closing in favor of clean PR without unrelated changes |
Additional Comments (2)
|
Summary
Addresses the TODO in backup.js to switch from synchronous file system operations to async fs.promises for better performance and non-blocking I/O.
Changes
getBackupDir()to async usingaccess()andmkdir()createBackup()to async usingwriteFile()andstat()parseBackupMeta()to async usingstat()Benefits
Verification
Closes TODO from CODE_IMPROVEMENTS.md analysis